-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce utils.simulate() #348
base: dev
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #348 +/- ##
==========================================
- Coverage 88.37% 87.97% -0.41%
==========================================
Files 13 13
Lines 3012 3051 +39
==========================================
+ Hits 2662 2684 +22
- Misses 350 367 +17
Continue to review full report in Codecov by Sentry.
|
I have a semi-related question because you are introducing What I don't like about using In my previous work, my solution was to use the information in the par-files for this. The uncertainty column in the par-file is currently only used as output, even though libstempo and PINT do read it in. Actually, I'm not sure whether PINT can use it for anything else. But it's a perfect place in the par-file to write the prior width as input. I mean, the parameter values in the par-file are in reality also just the means of the prior in the analysis. People just haven't been thinking about it like that, but that's what it is. I'd also love to have a discussion about this with the timers. Ideally the timing group(s) would create data releases with more physical priors included on all parameters. So my suggestion is to use the uncertainty column in the par-file to specify the prior width. |
Two more questions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks ok. I provided minor code feedback.
Question:
In the unit tests, right now the returned residuals are not actually checked, which makes the tests incomplete. Ideally we would generate an ensemble of realizations and compare that to the covariance matrix. That would be most realistic. It also sounds less feasible.
Also, my question regarding the timing model parameter priors still stand. IMO those should actually come from the timing package. I don't want to make that a show-stopper though.
Any thoughts?
inv = sl.cho_solve(cf, np.identity(cf[0].shape[0])) | ||
if logdet: | ||
ld = 2.0 * np.sum(np.log(np.diag(cf[0]))) | ||
except np.linalg.LinAlgError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a # pragma: no cover here?
gpresiduals.append(0) | ||
elif phi.ndim == 1: | ||
gpresiduals.append(np.dot(fmat, np.sqrt(phi) * np.random.randn(phi.shape[0]))) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a # pragma: no cover here
for delay, ndiag in zip(delays, ndiags): | ||
if ndiag is None: | ||
whiteresiduals.append(0) | ||
elif isinstance(ndiag, ShermanMorrison): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a good idea to check for an instance of ShermanMorrison. When 'fastshermanmorrison' is used, this will be a different type. Instead, perhaps duck typing can be used, like:
if all(hasattr(ndiag, attr) for attr in ['_nvec', '_jvec', '_slices']):
cf = cholesky(sps.csc_matrix(phis)) | ||
gp = np.zeros(phis.shape[0]) | ||
gp[cf.P()] = np.dot(cf.L().toarray(), np.random.randn(phis.shape[0])) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a #pragma: no cover
simulate
will draw random residuals for all pulsars inpta
by sampling all white-noise and GP objects for parametersparams
.combine=False
, and will run faster with GP ECORR.pta
includes aTimingModel
, that should be created with a smallprior_variance
. Note that the variance applies to the entire design-matrix vector, and should be multiplied bylen(psr.toas)
if it is understood as the variance per individual residual.utils.set_residuals
to replace residuals in aPulsar
object.parameter.sample(pta.params)
to sample from the prior.Pulsar
may nevertheless cache residuals internally, so it is safer to rebuild the PTA with the modifiedPulsar
.